Skip to content

Conversation

@altendky
Copy link
Contributor

@altendky altendky commented Jan 16, 2021

Draft for:

  • Conclusion on the flake8 status
    • Should it be failing with thousands of errors? Or am I running it wrong...
    • Presently it is set to allow failure. This allows us to see the output without having it's failure mask the failure of the rest of the build. Travis wasn't ever actually running flake8 so I think this is acceptable for an initial switch to GitHub Actions.
  • Conclusion on Coveralls
    • Coveralls doesn't support PRs from forks via GitHub Actions. One solution is to just put the Coveralls repository token into the repository. Trio and Twisted have done similar, for example, with the reasoning being that an evil actor uploading coverage reports isn't the scariest of attack vectors for a project. Or, if you aren't super attached to Coveralls, Codecov has a GitHub app that deals with it.
    • Coveralls will be added in 596.

This is just to confirm it is running before adding lots of config (presumably with errors...).
@altendky altendky marked this pull request as draft January 16, 2021 14:06
@dhoomakethu
Copy link
Contributor

@altendky this looks good. Would you mind take a look in to the failure cases please?

@altendky
Copy link
Contributor Author

@dhoomakethu, yep, working on it. Note that the PR is marked draft as it is not ready. :]

@dhoomakethu
Copy link
Contributor

looks like the tests failed on linux for missing .coverage file and on windows. Btw, its good to have windows tests in place but I can live with out it for the time being.

@altendky
Copy link
Contributor Author

Yes, I tossed them all in. I'll see what I can get working by just correcting the CI test setup as opposed to what needs 'real' fixes that should be addressed in a separate PR.

@dhoomakethu
Copy link
Contributor

I have merged all your other PR's in to branch 2.5.0 and are now part of #578 . Thanks for the contributions.

@altendky altendky mentioned this pull request Jan 16, 2021
They can be enabled in separate PRs that fix their issues.
This was referenced Jan 16, 2021
@altendky
Copy link
Contributor Author

Getting closer, need to review some various tidbits and todos...

@altendky
Copy link
Contributor Author

@dhoomakethu, alrighty, as listed in the first comment there are two reasons I have this still as a draft. It's almost there except for the flake8 and Coveralls questions. I am fine discussion them on either Gitter or here, whatever you like. Also, there are follow-up PRs for enabling Windows (#593) and macOS (#594) testing. They may take a bit more 'real code' changes. The code changes I included here seemed trivial enough to be ok.

If any of this doesn't follow the spirit of the pymodbus project setup, don't hesitate to mention it. When I start using new projects I tend to end up doing various cleanup (like this CI PR) and try to get a feel for how the project likes to operate and be configured etc. I'll have my opinions, but I can certainly make adjustments to make my changes fit better. :]

@altendky
Copy link
Contributor Author

@dhoomakethu, alrighty, I think this is at least worth looking over now. One thing that GitHub Actions is lacking is yaml anchors or other tooling for reusing various bits. A result of this is a fair bit of duplication between the job matrices. I don't like this, I have accepted it after the various setups I have done. The extensive and sometimes repetitive definitions are another thing I have accepted that aren't representative of my normal approach.

          - name: CPython 2.7
            tox: py27
            action: 2.7
            docker: 2.7
            implementation: cpython

I've concluded that it is simpler to just repeat each needed value there for it's target use. Admittedly, I often work with more complicated matrices (os/python version/python interpreter/qt library/qt version/and sometimes another one or two...) and end up applying this sometimes beyond what is needed in the simpler cases. Also, some of the jobs in the config could be made simpler but I have chosen to keep them similar to each other instead.

The Docker images for Linux are a habit I've made after I ran into some issues that were _only_ reproducible in GitHub Actions native Linux VMs. If GHA supported Windows and macOS Docker images I would probably do that too. But, it's likely not important here.

Anyways, given that I am new to this project I thought I'd give some context for this PR. I am happy to make changes or further discuss any topics in this or any other venue you prefer.

@altendky altendky marked this pull request as ready for review January 20, 2021 19:38
@altendky
Copy link
Contributor Author

Kick CI to make sure this is still working 'well'.

@altendky altendky closed this Jan 27, 2021
@altendky altendky reopened this Jan 27, 2021
@altendky
Copy link
Contributor Author

I would generally recommend lowering the limit for coverage so that it passes. Using a global coverage limit to mark a given PR as passing or failing isn't super useful. It doesn't indicate if that PR has successfully covered their own changes or not so it says very little about the PR. Rather, it blocks the indication of success on every other front. Codecov (and I assume coveralls) can provide a status for the coverage of the patch which does speak directly to the PR.

That said, if you want increased coverage then you can create a PR to bump the required coverage up to whatever level you want and work on it until you surpass that level.

@altendky
Copy link
Contributor Author

Looks like there is a small thing to fix.

@altendky altendky marked this pull request as draft January 27, 2021 15:20
@altendky altendky marked this pull request as ready for review January 27, 2021 16:04
@altendky altendky mentioned this pull request Feb 2, 2021
5 tasks
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 9, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@altendky
Copy link
Contributor Author

altendky commented Feb 9, 2021

@dhoomakethu, is there anything I should work on here? If not I can be patient until you have time. :]

@dhoomakethu
Copy link
Contributor

@altendky, the tests on 3.7 are failing not sure if thats Ok, I was holding this up because of that. Thought you were working on it.

@dhoomakethu
Copy link
Contributor

never mind, I have the fix for that. I will merge this in to dev this week. Thanks for your patience.

@dhoomakethu
Copy link
Contributor

@altendky this is merged and is part of #605. I will be closing this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants